Improve xdr cython #3892
Improve xdr cython #3892
Conversation
Codecov ReportBase: 94.39% // Head: 94.38% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #3892 +/- ##
===========================================
- Coverage 94.39% 94.38% -0.02%
===========================================
Files 194 194
Lines 25152 25226 +74
Branches 3399 3402 +3
===========================================
+ Hits 23743 23809 +66
- Misses 1360 1368 +8
Partials 49 49
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
@hmacdope does this get rid of the yellow lines from the annotation? One lazy way of smoking out Python bits is to put code blocks into |
|
This is ready for a first review. Reading directly into a memoryview of As I see it the main remaining problem is in the This is a bit of a chicken and eg problem though and I don't see an easy way around this without the ability to "read" from a TRR frame whether there are forces, velocities present etc. I had a browse of the XDR source but couldn't see a flag that readily indicates presence or absence of data, Query @tylerjereddy @kain88-de as to whether it may be possible to read some kind of flagged state from the file? Cheers |
|
There is no flag known to me, the format is really basic. However, we already generated a map of the different frames to support random seeks. Nothing is stopping us from extending that map to also include information about the velocities and forces for each frame. |
Okay, thanks for that. That was my understanding also that there were no flags. Thanks for confirming. |
|
This now has a "fat timestep" with positions, velocities and forces at every frame, as the arrays are allocated anyway in reading. These are then read using memoryviews. Its about 5-8% more efficient in my testing. |
|
@orbeckst I have added an I may have to |
| * Improve C content of libxdr Cython, add `read_direct` methods to read | ||
| coordinates, velocities and forces directly into memoryviews of `Timestep` | ||
| attributes, make `TRR` timestep have positions, velocities and forces on | ||
| `__init__`. (Issue #3891 PR #3892). |
There was a problem hiding this comment.
add under Fixes: XTC and TRR readers now fail with IOError when a status except EOK (=0) is detected on reading a frame instead of silently continuing
There was a problem hiding this comment.
Some readers had enough redundancy built in to avoid not continuing with retcode != EOK but at least its now done consistently.
There was a problem hiding this comment.
This was the pattern that protected some branches
if return_code != EOK and return_code != EENDOFFILE \
and return_code != EINTEGER:
raise IOError('TRR read error = {}'.format(
**error_message[return_code]))**
if return_code == EENDOFFILE or return_code == EINTEGER:
self.reached_eof = True
raise StopIteration
# can only be EOK at this point but it was a bit opaque. There was a problem hiding this comment.
Ok, good to know that we probably do not change a lot of behavior. Nevertheless, the current approach is cleaner.
(Btw, while somewhere I came across my own explanation of why TRR stops with EINTEGER and I think it was related to not being able to read the magic number.)
|
@richardjgowers I'm going to make the executive decision to not implement buffers, because they are only relevant for methods that we plan to deprecate? Thoughts? LMK if you would prefer to do it. |
|
@hmacdope yeah, because the buffers we're using are already the Timestep ones right? That sounds fine |
|
Should be good to go for a squash-merge. |
…nalysis#3939) * ensure that the same LICENSE file appears everywhere * update CHANGELOG
|
With two approvals I will go ahead and merge. :) |
Fixes #3891
Changes made in this Pull Request:
PR Checklist